Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add support for copying contextvars to thread workers #389

Closed
wants to merge 3 commits into from

Conversation

tiangolo
Copy link
Contributor

✨ Add support for copying contextvars to thread workers.

This adds the necessary logic to support copying the context in asyncio. There's an equivalent, supporting Trio PR here: python-trio/trio#2160

This is related to #363

This also includes the workaround for Trio for what I can do here on AnyIO (which wouldn't be necessary with python-trio/trio#2160). But there's the particular case that I can't make work here without python-trio/trio#2160, of calling from_thread.run (with an async function).

I don't know what's preferred, if having the extra workaround for Trio here or waiting for that PR in Trio and then removing the extra code here, but require a higher Trio version (once that's released). Let me know if I should remove all that.

Testing locally with that Trio branch the new tests seem to pass.


For completeness, here's the relevant bug in Python: https://bugs.python.org/issue34014 where the authors (asyncio, async, await) seem to agree that the context should be copied, but it was unfeasible to change in asyncio.


I want to get some feedback on this before updating docs, etc. 🤓

@agronholm
Copy link
Owner

I wish you had reached out to me first. I have a nearly complete PR of my own coming, just missing one BlockingPortal fix.

@agronholm
Copy link
Owner

This PR is quite different from what I have locally. I will push my changes and make a PR tomorrow so we can compare notes.

@tiangolo
Copy link
Contributor Author

Sure thing!

And sorry, I did most of the work on a long flight a couple of weeks ago without internet (and I was mostly offline for the past two weeks). And the main part of my work was just porting my tests from the other Trio PR.

If we can check that the tests/use cases pass with your PR I'm perfectly fine with closing mine. 🙂

Also feel free to just copy paste anything you find useful here in this PR, no need to worry about "commit attributions" or anything like that. 🤓

@agronholm
Copy link
Owner

Your PR seems to handle things better than mine. I had some ugly workarounds for trio but I'd rather have trio solve those problems on their end while I deal with asyncio here. I'll look into merging our efforts into one conclusive PR, with BlockingPortal also covered and without the trio workarounds.

@tiangolo
Copy link
Contributor Author

Awesome, thank you! Feel free to close this PR whenever you consider it appropriate.

@agronholm
Copy link
Owner

I have my own PR now (#390) which incorporates tricks from this one. We still need trio to fix the problem on their end.

One unresolved issue is what sniffio.current_async_library() should give in a worker thread. In my opinion it should raise AsyncLibraryNotFoundError since it's not in an event loop thread.

@smurfix
Copy link
Collaborator

smurfix commented Nov 18, 2021

I'm +1 on AsyncLibraryNotFoundError in principle.

On the other hand, the cases where that actually matters (i.e. you use a library within the thread that uses sniffio) are likely to be few and far between. Thus I'd be OK with requiring the user to explicitly clear sniffio's state if that's required.

@agronholm
Copy link
Owner

Looking at @tiangolo 's trio PR, I see that it is setting the current async library to None, but my test still fails on trio:

async def test_asynclib_detection() -> None:
    with pytest.raises(sniffio.AsyncLibraryNotFoundError):
        await to_thread.run_sync(sniffio.current_async_library)

@agronholm
Copy link
Owner

What I think is going on here (correct me if I'm wrong) is: the cached name of the current async library is voided, but the trio token is still present in the context so sniffio's detection picks that up and declares that the worker thread is running on the trio event loop.

@smurfix
Copy link
Collaborator

smurfix commented Nov 18, 2021

? sniffio doesn't check for Trio. Like, at all. Trio explicitly sets sniffio's contextvar.

@agronholm
Copy link
Owner

Right. So why is this happening then?

@smurfix
Copy link
Collaborator

smurfix commented Nov 18, 2021

I suspect that to_thread.run_sync uses some context to run the thread … just not the one you're calling to_thread.run_sync from. As Trio's thread engine does thread caching AFAIR, that seems somewhat likely at least.

@agronholm
Copy link
Owner

Ahh...it's anyio that's setting sniffio's contextvar in the worker thread. I think that's a no-no, and I need to use my own context for that instead.

@agronholm
Copy link
Owner

Removed the lines that set the sniffio cvar; no ill effects noted in the test suite. The PR is now ready, once trio is. We should continue the discussion over at #390.

@agronholm agronholm closed this Nov 18, 2021
@tiangolo tiangolo deleted the contextvars-threads branch November 29, 2021 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants